fix(restore-session): scrub Nostr keys from log lines by ToRyVand · Pull Request #835 · MostroP2P/mostro

ToRyVand · 2026-07-21T00:40:22Z

Problem

AGENTS.md:48 says: "Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed."

src/app/restore_session.rs logged master_key/trade_key verbatim in 3 places (tracing::info!/tracing::warn!). master_key in particular is the user's persistent Nostr identity, not a per-trade ephemeral key — every successful restore-session request hits this path in production (the hex-format validation guards always pass for real PublicKey values, so this isn't a rare branch). If these logs reach an external aggregator, anyone with log access can correlate "this identity restored a session at this timestamp" in cleartext.

Solution

Dropped the key from all 3 log lines. The surrounding message (action + outcome) is still useful for debugging without the identifying value.

Tests

No test asserted on log content before or after — the fix only removes an interpolated value from 3 tracing macro calls, no behavior change. Full suite still green: cargo test 1019/1020 passing (the 1 failure is a pre-existing, unrelated local-environment issue — a fixed-port test server colliding with something already bound to :8080, same as on other recent PRs).

cargo fmt --check and cargo clippy --all-targets --all-features -- -D warnings both clean.

Closes #834

Summary by CodeRabbit

Bug Fixes
- Removed sensitive key information from restore-session logs.
- Restore-session processing and error handling remain unchanged.

master_key/trade_key were logged verbatim in 3 tracing calls, violating AGENTS.md's "scrub logs that might leak invoices or Nostr keys" guideline. master_key in particular is the user's persistent Nostr identity, not a per-trade ephemeral key — if these info/warn logs reach an external aggregator, anyone with log access could correlate "this identity restored a session at this timestamp" in cleartext. Closes MostroP2P#834

coderabbitai · 2026-07-21T00:40:42Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8f1882c4-ecbb-460d-8f6e-a43cf4ec4353

📥 Commits

Reviewing files that changed from the base of the PR and between 57b8ff5 and 055b5f4.

📒 Files selected for processing (1)

src/app/restore_session.rs

Walkthrough

Restore-session logging was updated to remove master_key and trade_key values from start, response, and timeout messages. Session processing, message handling, and error behavior are unchanged.

Changes

Restore-session logging

Layer / File(s)	Summary
Scrub restore-session logs `src/app/restore_session.rs`	Restore start, response-sent, and timeout logs no longer include Nostr key values and include log-scrubbing notes.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Possibly related PRs

MostroP2P/mostro#826: Both changes touch src/app/restore_session.rs, including restore-session validation and logging-related code.

Suggested reviewers: grunch

Poem

I’m a rabbit with keys tucked away,
No secrets in logs at the end of the day.
Restore starts clean,
Responses stay serene,
Timeout trails safely hop away.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: removing Nostr keys from restore-session logs.
Linked Issues check	✅ Passed	The PR removes all three cleartext key logs required by issue `#834` and leaves behavior unchanged.
Out of Scope Changes check	✅ Passed	No unrelated code changes are described; the patch stays focused on log scrubbing in restore_session.rs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

arkanoider · 2026-07-21T07:32:54Z

@grunch what do you think? We can remove that printout of keys I suppose. It's a good practise also in the restore case imo.

grunch

LGTM

grunch · 2026-07-23T15:23:31Z

thank you very much @ToRyVand good catch 😄

ToRyVand · 2026-07-24T02:19:27Z

Merged — technical summary

Merged by @grunch in 2626941. Recap for the record:

Problem. AGENTS.md requires scrubbing logs that could leak invoices or Nostr keys. restore_session.rs logged master_key/trade_key verbatim in 3 places — master_key is the user's persistent Nostr identity, and the path runs on every successful restore-session request in production.

How it was found. Not from the backlog — surfaced while reviewing CodeRabbit's post-rebase re-review of #826. Verified the lines were pre-existing on main (untouched by #826's own diff) before filing #834 publicly; confirmed no SECURITY.md/disclosure policy exists in the repo and that the values are Nostr public keys, not secrets, so a public issue was the right channel.

Fix. Dropped the key from all 3 log sites, replacing with an inline comment citing the AGENTS.md rule at each site rather than one comment at the top of the file — keeps the reasoning visible exactly where someone might be tempted to re-add the value for debugging.

Follow-up filed, not scope-crept in: a review pass on this fix found the identical pattern unaddressed in 5 more files (scheduler.rs, app.rs ×2, last_trade_index.rs, db.rs). Rather than expand this PR, opened #836 proposing a structural fix (redaction layer / lint) instead of 5 more one-off patches — left unclaimed for maintainer input.

Final: 1 commit, 1 file, +8/-6. cargo fmt/clippy -D warnings/cargo test clean.

Closes #834.

ToRyVand mentioned this pull request Jul 21, 2026

Nostr keys logged in cleartext across multiple modules — needs a structural fix, not per-line patches #836

Open

grunch approved these changes Jul 23, 2026

View reviewed changes

grunch merged commit 2626941 into MostroP2P:main Jul 23, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(restore-session): scrub Nostr keys from log lines#835

fix(restore-session): scrub Nostr keys from log lines#835
grunch merged 1 commit into
MostroP2P:mainfrom
ToRyVand:fix/834-scrub-nostr-keys-from-logs

ToRyVand commented Jul 21, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

arkanoider commented Jul 21, 2026

Uh oh!

grunch left a comment

Uh oh!

Uh oh!

grunch commented Jul 23, 2026

Uh oh!

ToRyVand commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ToRyVand commented Jul 21, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

arkanoider commented Jul 21, 2026

Uh oh!

grunch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grunch commented Jul 23, 2026

Uh oh!

ToRyVand commented Jul 24, 2026

Merged — technical summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ToRyVand commented Jul 21, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading